Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Python example for PrintOptions by replacing scale with page_height and page_width #2122

Merged
merged 2 commits into from
Jan 5, 2025
Merged

Fix Python example for PrintOptions by replacing scale with page_height and page_width #2122

merged 2 commits into from
Jan 5, 2025
+2 −2

Conversation

yvsvarma
Copy link
Contributor

@yvsvarma yvsvarma commented Jan 5, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Description

This PR updates the Python example for PrintOptions by replacing the incorrect use of the scale attribute with the appropriate methods (page_height and page_width) based on the current Selenium repository implementation.

Motivation and Context

A previous commit addressed the Java example for PrintOptions in PR #2118. This PR extends that work to Python, addressing issue #2095, where a user specifically requested a corrected Python example. The updated example demonstrates the correct use of PrintOptions for setting page dimensions.

Types of Changes

Checklist

  • I have read the contributing guidelines.
  • I have used hugo to render the site/docs locally and confirm they look as expected.

PR Type

Bug fix, Documentation

Description

  • Fixed Python example for PrintOptions to correctly demonstrate page_height and page_width.

  • Replaced incorrect scale usage with page_height in the test_size function.

  • Addressed user feedback from issue [🐛 Bug]: No size example, two scale examples #2095 regarding missing PaperSize example.

  • Improved documentation consistency with Selenium repository implementation.

Changes walkthrough 📝

Relevant files
Bug fix
test_print_options.py
Updated `test_size` to use `page_height` instead of `scale`

examples/python/tests/interactions/test_print_options.py

  • Replaced scale with page_height in the test_size function.
  • Added an assertion for page_height to validate the change.
    		• +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @netlify Netlify
    Copy link

    netlify bot commented Jan 5, 2025 •
    edited
    Loading

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit bca0efa

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    2095 - Partially compliant

    Compliant requirements:

    • Remove duplicate scale example
    • Provide correct example for setting page size

    Non-compliant requirements:

    • The example still doesn't show how to set PaperSize property as requested in the original issue

    Requires further human verification:

    • Verify if the page_height value of 27.94 is a reasonable default value for the example
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Fix

    The comment "Use page_width to assign width" is misleading as no page_width is actually set in the example

    print_options.page_height = 27.94  # Use page_width to assign width

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Enhance test coverage by validating both height and width parameters

    The test function named test_size is now only testing page height, but should also
    test page width to fully validate page size functionality.

    examples/python/tests/interactions/test_print_options.py [23-27]

     def test_size(driver):
     driver.get("https://www.selenium.dev/")
     print_options = PrintOptions()
-    print_options.page_height = 27.94  # Use page_width to assign width
+    print_options.page_height = 27.94
+    print_options.page_width = 21.59
     assert print_options.page_height == 27.94
+    assert print_options.page_width == 21.59
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves test coverage by adding validation for page_width alongside page_height, making the test more comprehensive and better aligned with its 'test_size' name.

    		8
    Fix misleading comment that references wrong property name

    The comment incorrectly mentions page_width while the code is setting page_height.
    Update the comment to accurately reflect the property being set.

    examples/python/tests/interactions/test_print_options.py [26]

    -print_options.page_height = 27.94  # Use page_width to assign width
+print_options.page_height = 27.94  # Set page height in centimeters
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The current comment is misleading as it mentions page_width while setting page_height, which could confuse developers. Fixing this improves code clarity and prevents potential misunderstandings.

    		7

    harsha509
    harsha509 approved these changes Jan 5, 2025
    View reviewed changes
    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Good catch 🚀
    Thank you @yvsvarma !

    @harsha509 harsha509 merged commit 215adae into SeleniumHQ:trunk Jan 5, 2025
    7 of 9 checks passed
    selenium-ci added a commit that referenced this pull request Jan 5, 2025
    @selenium-ci
    Fix Python example for PrintOptions by replacing scale with page_heig…
    acb07b0 
    …ht and page_width (#2122)[deploy site]

Fix Python example for PrintOptions to address issue #2095

Co-authored-by: Sri Harsha <[email protected]> 215adae
    @yvsvarma
    Copy link
    Contributor Author

    yvsvarma commented Jan 5, 2025

    Good catch 🚀 Thank you @yvsvarma !

    Thank you for your time in reviewing Sri! @harsha509

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @harsha509 harsha509 harsha509 approved these changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @yvsvarma @harsha509